[Open Data] Fix ISF ratio scale, region coverage and culture handling, and keep retired-SKU reservations visible - #2308
Conversation
The Catalogs API only returns SKUs that can still be purchased, so the instance size flexibility dataset dropped every retired series that active reservations still cover (Av2, D, DS, Dv2, Dv3, Ev3, F, G, H, LS, NC, NV and others). The Optimization Engine joins ISF with kind=inner, so those reservations were silently missing from its reports. - Backfill the retired groups from the ratio file the API replaced: 1,353 -> 2,573 SKUs and 211 -> 318 flexibility groups. Purely additive against v15 -- no row removed and no published ratio changed. - Merge additively instead of overwriting, so a SKU the API stops returning is carried forward rather than dropped, and fail the run if the dataset would shrink. - Enumerate every physical region from the ARM locations API (63) instead of sweeping a hardcoded list of 26, which omitted SKUs that launch in only a handful of regions. regionType is nested under metadata, and the logical groupings it marks are not valid catalog scopes. - Reconcile the ratio scale. The retired file normalized each group so its smallest SKU was 1 while the API reports vCPU counts, and because Azure retires individual sizes the API covers only part of 44 of the 204 shared groups, so a per-SKU merge left single groups holding both units. Backfilled rows were converted onto the API scale once, and Assert-IsfScale now fails the run if a group drifts off it again. - Drop non-positive ratios. The API returns one, and the benefits simulation workbook divides by Ratio. Fixes #2300 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
There was a problem hiding this comment.
🟡 Changes recommended
The generator uses culture-dependent [double]::TryParse(...) for ratios, which can misparse decimal values in non-dot locales and jeopardize dataset correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes gaps in the Instance Size Flexibility (ISF) open-data pipeline by making the generated dataset additive (carrying forward retired-but-still-relevant SKUs), expanding region coverage via ARM locations enumeration, and enforcing invariants around ratio scale and non-positive ratios to prevent downstream consumer errors.
Changes:
- Merge Catalogs API results over the previously published ISF CSV keyed by
ArmSkuName, and fail the run if the dataset would shrink. - Enumerate all physical Azure regions from the ARM locations API when
-Locationis not provided, replacing the hardcoded region list. - Drop non-positive ratios, add scale-consistency validation (
Assert-IsfScale), expand unit tests, and update documentation/changelog to reflect the new behavior.
File summaries
| File | Description |
|---|---|
| src/scripts/Update-InstanceSizeFlexibility.ps1 | Implements additive merge, ARM-based physical region enumeration, ratio filtering, and scale validation logic. |
| src/powershell/Tests/Unit/Update-InstanceSizeFlexibility.Tests.ps1 | Adds unit tests for additive merge behavior, zero-ratio filtering, scale mismatch detection, and region enumeration paths. |
| src/open-data/README.md | Updates dataset behavior documentation (additive merge, region coverage, ratio scale notes, zero-ratio handling). |
| src/open-data/InstanceSizeFlexibility.csv | Backfills and expands the published ISF dataset with retired SKUs and additional groups. |
| docs-mslearn/toolkit/open-data.md | Updates Microsoft Learn documentation to reflect additive dataset behavior, region coverage, and ratio scale handling. |
| docs-mslearn/toolkit/changelog.md | Adds changelog entries describing the fixes and dataset growth. |
| .github/workflows/opendata-instance-size-flexibility.yml | Updates workflow PR body text to align with additive/merge semantics and expected diffs. |
Review details
Suppressed comments (2)
src/scripts/Update-InstanceSizeFlexibility.ps1:337
- This ratio parse also defaults to the current culture. Since the published CSV uses
.for decimals, cultures that expect,can misparse or reject values, which would affect the duplicate-group detection and merge inputs. UseInvariantCulturefor deterministic behavior.
{
$ratio = 0.0
if (-not [double]::TryParse($row.Ratio, [ref]$ratio)) { continue }
if ($ratio -le 0) { continue }
src/scripts/Update-InstanceSizeFlexibility.ps1:346
- Same culture-dependent parsing issue here: using the current culture can mis-handle decimal ratios in the CSV (which are written with
.). UseInvariantCultureto avoid silently changing which rows are retained during the merge.
foreach ($row in $rows)
{
$ratio = 0.0
if (-not [double]::TryParse($row.Ratio, [ref]$ratio)) { continue }
if ($ratio -le 0) { continue }
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
The ratio-scale follow-up flagged in the description is now tracked separately in #2309, so it doesn't hold up this PR. That one is about the Optimization Engine's math; this PR leaves every published ratio unchanged. |
The ratios were described as absolute vCPU counts. That holds for many D/E-series groups by coincidence -- their smallest SKU has 2 vCPUs, so normalizing against it yields a factor of 2 -- but not in general: - 799 of 1,164 parseable VM SKUs match their vCPU count; 365 do not. - Standard_B2as_v2 has 2 vCPUs and a ratio of 16; Standard_B16als_v2 has 16 and a ratio of 113.4. - azure_managed_redis_balanced_b1000 has a ratio of 1248. - 156 of the returned ratios are not integers. What is actually verifiable is that the API leaves ratios unnormalized: only 52 of its 211 groups start at 1, against 432 of 433 in the retired files. That is what the merge and the scale check rely on, so the wording now states it and no longer names a unit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Depends on merge order with #2298, which adds This PR takes the CSV to 2,573 rows, so that generated file has to be rebuilt. It is deliberately not included here: both PRs would otherwise add the same new file and collide. Plan: let #2298 merge first, then merge Note that Open Data CI passing on this PR does not mean the file was regenerated — on Hub KQL is unaffected: |
Ratios were parsed and written with the current culture. On a comma-decimal machine that corrupts the dataset in both directions: - de-DE parses "2.1" as 21, so a ratio is silently ten times too large. - fr-CH fails the parse outright, so the row is dropped. - Export-Csv writes the value back as "2,1", which breaks every consumer: the Optimization Engine's externaldata(... Ratio:double) and the Power BI partitions typed as number. CI runs on ubuntu-latest and is unaffected, but the README documents running the generator by hand, so a contributor in a comma-decimal locale would publish a corrupt file. Both parse sites and the write path now pin InvariantCulture, with a regression test that drives the generator under de-DE and asserts the round trip. Also corrects the SKU count in the changelog: it still said 2,574 from before the zero-ratio row was dropped, against 2,573 actually published. Reported by Copilot review on #2308 (the parse side; the write side turned up while verifying it). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… fixes Reverts the backfill of retired SKUs and everything it required: the additive merge, the shrink guard, Import-IsfCsv, Assert-IsfScale and the one-time migration onto the API scale. The generator publishes exactly what the Catalogs API returns again, and aborts without touching the file if any scope fails. The premise behind the backfill does not hold up. The dataset would have carried 1,220 rows sourced from a file Microsoft has retired, frozen forever because nothing can refresh or correct them, in a dataset whose contract is a weekly refresh from the authoritative API. Reservations on retired SKUs are better handled where they surface -- see #2300 for the Optimization Engine side. What remains are three independent defects, all of which concern the purchasable SKUs the API does return: - Region coverage: the sweep used a hardcoded list of 26 regions, so SKUs that launch in only a few regions were missing. It now enumerates every physical region from the ARM locations API (63 today). regionType is nested under metadata, and the logical groupings it marks are not valid catalog scopes. - Culture: ratios were parsed and written with the current culture, so a comma-decimal machine read "2.1" as 21 or rejected it, and wrote "2,1" back into the published file. - Zero ratios: the API returns one, and the benefits simulation workbook divides by Ratio. The published dataset goes from 1,353 to 1,363 SKUs -- no row removed, ten added by the wider region sweep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reservation workbooks join the instance size flexibility open data with
kind=inner. The Catalogs API that feeds it only returns SKUs that can still
be purchased, so an active reservation on a retired size has no row to match
and is dropped from the report entirely -- not shown at zero, absent. The
customer keeps paying for a reservation the tool stops mentioning.
Reservations run one or three years and outlive the sellability of their
size, so this is reachable in normal use.
All seven ISF joins in reservations-usage.json become leftouter, with the
group and ratio falling back to the SKU itself:
| extend ISFGroup = coalesce(ISFGroup, SKUName_s), Ratio = coalesce(Ratio, 1.0)
Without that fallback a plain leftouter is worse than the current behaviour:
Ratio and ISFGroup come back null, every unmatched reservation collapses into
one null bucket, and the utilization column reads empty.
Ratio 1 is correct for these rows. Util7Days_s comes from Azure and already
accounts for flexibility, so the percentage is right; the ratio only converts
between group units, which is a no-op for a single-SKU group.
reservations-potential.json and benefits-simulation.json keep kind=inner.
They model what to buy, where restricting to purchasable SKUs is correct.
Also points the changelog entries for the culture and zero-ratio fixes at
this PR rather than #2300, which they have nothing to do with.
Note the workbook KQL is not executed by any test in this repo, so this
change is reasoned rather than verified.
Fixes #2300
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Catalogs API leaves ratios unnormalized -- only 52 of its 211 groups have a smallest SKU of 1, which Microsoft's documentation calls out (BS Series starts at 0.25, Ddsv5 Series at 2) alongside a normalization step of its own. The retired isfratioblob.csv this dataset replaced was normalized, so #2222 silently changed the scale under every consumer. That matters because the Optimization Engine converts quantities into units of a group's smallest SKU -- AvgRIsUsedInSmallestRatio is the variable's own name -- and multiplying by a ratio that doesn't start at 1 inflates the absolute figures by a per-group constant. Utilization percentages carry the factor on both sides and were never wrong; the displayed quantities were. Normalizing loses nothing: ratios are only meaningful within their group, and both forms carry identical proportions. It also makes the leftouter fix in this PR coherent. An unmatched reservation falls back to Ratio 1, which is implicitly "smallest SKU = 1"; next to an unnormalized group that would put two rows of one table in different units. Against the retired file, 1,223 of the 1,291 rows they share now agree. The 68 that don't split cleanly: - 12 groups / 52 rows where the API no longer returns the historically smallest SKU, so normalization anchors on a different one. BS Series High Memory is down to Standard_B20ms alone, which normalizes to 1 rather than 40.2. Internally consistent, since no other SKU of that group remains. - 11 groups / 16 rows, all dedicated host, where the API and the retired file disagree on the proportions themselves rather than the unit. The API is the newer source. -Normalize is replaced by -Raw, so the switch names the departure from the default rather than the default itself. Fixes #2309 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hélder Pinto (helderpinto)
left a comment
There was a problem hiding this comment.
The PR description should be updated to remove the "Fixes #2309" statement, because it does not address the so called ratio scale issue.
| - It covers Virtual Machines, Redis Cache, and Dedicated Host across every physical Azure region. | ||
| - `ArmSkuName` is unique across the file and can be used as a join key on its own. | ||
| - Ratios are the raw Microsoft values within each group (the smallest SKU isn't always `1`). | ||
| - Ratios are normalized so the smallest SKU in each flexibility group has a ratio of `1`. Compare ratios only within a flexibility group. |
There was a problem hiding this comment.
I am not sure whether generating a CSV whose ISF ratios are inconsistent with what Cost Management exports report in the RINormalizationRatio property of the AdditionalInfo column is a good idea.
There was a problem hiding this comment.
I am not seeing the full picture, but once I address #2309 I'll have a better idea.
Michael Flanakin (flanakin)
left a comment
There was a problem hiding this comment.
🤖 [AI][Claude Code] PR Review
Summary: Strong, well-reasoned PR. The generator fixes (invariant culture, region enumeration, non-positive ratio drop, normalization) are correct and well tested, and the leftouter + coalesce pattern is the only safe form here — KQL strings have no null, so leftouter yields "" and coalesce (documented as "first non-null, or non-empty for string") handles both the string and the double case where isnull() would silently fail. Verified locally: 23 unit tests pass (the description says 22), all 3,593 lint tests pass, and the published CSV holds every claimed invariant — 1,363 rows, 213 groups, every group normalized to a smallest ratio of 1, ArmSkuName globally unique, no non-positive ratios, invariant decimal formatting. The Power BI claim also checks out: x_CommitmentDiscountFlexRatio appears only in linguistic metadata, in no DAX measure or visual.
One blocker: the fix stops three joins short of complete, and the reported symptom survives in one of them.
🚫 Blockers (1)
reservations-usage.jsonhas 10 ISF joins, not 7. Three were alreadyleftouterbefore this PR and still have nocoalesceguard — including "Unused Reservations over time (by VM count)" (line 1093), which multiplies by a nullRatioand drops retired-SKU reservations exactly as #2300 describes.
⚠️ Should fix (3)
- The Optimization Engine changelog section now has two separate
- **Fixed**blocks, the new one placed before- **Added**. - The instance size flexibility sample table in
open-data.mdshows rows that exist nowhere in the published file. leftouterwidens the two ISF-grouped tables to every reservation type, with dead-end drilldowns — an undocumented scope change.
💡 Suggestions (1)
- The ARM locations call has no retry, unlike every Catalogs call.
Not verified, and I couldn't verify it either: no test in this repo executes workbook KQL, so the runtime behavior of these queries is reasoned, not run. Everything I checked above is static analysis plus the Pester suites and the published CSV.
… types Addresses review feedback on #2308. - Add the coalesce guard to the three ISF joins that were already leftouter before this PR and so never appeared in the diff. Without it, a reservation on a retired SKU still contributed nothing to "Unused Reservations over time (by VM count)" with Use ISF = Yes, collapsed into a single mislabeled series in the by-SKU cost chart, and showed a blank ISF group in the reservations grid. All ten ISF joins are now leftouter and guarded. - Scope the unmatched-SKU fallback to the reserved resource types that have instance size flexibility, matching the types the ISF dataset covers. Types without it keep their prior behavior in the Use ISF = Yes views and remain fully listed in the Use ISF = No views. - Extract Invoke-ArmRequest so region enumeration retries transient failures like every other ARM call in the generator. It is the first call the weekly run makes and a prerequisite for all of them, so a single 429 or 5xx failed the whole refresh before it fetched a single catalog page. - Merge the duplicate Optimization Engine "Fixed" block, replace the instance size flexibility sample table with rows that exist in the file, and fold the new dataset's fix bullets into nested notes under "Added". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflict was ms.date in docs-mslearn/toolkit/changelog.md only, resolved to today per AGENTS.md. Both sides' entries are kept: the FinOps hubs ADF trigger fix from #2291 and the Optimization Engine and open data entries from this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback addressed — ready for another lookEverything from the 9 Sep review is in, in Michael Flanakin (@flanakin) — your approval on 9 Sep 20:19 predates the agent review you posted at 23:58 the same evening, so it doesn't cover the blocker or anything below. Re-approval would be appreciated. What changed
The one judgment call worth a second lookI scoped the fallback rather than documenting the widening. Two things that weren't in the original analysis pushed it that way: Nothing is hidden either way. Both affected grids sit in groups gated on The dead-end drilldown is confirmed, not inferred. The | extend VMSize = tostring(parse_json(AdditionalInfo_s).ServiceType)
| extend ConsumedSize = iif(isnotempty(VMSize), VMSize, strcat(MeterSubCategory_s, ' ', MeterName_s))That line exists because those rows are real. The ISF drilldown filters on Hélder Pinto (@helderpinto)Two things for you, since you're the only real-data check this PR has:
Verification
|
Summary
Fixes #2300 and #2309. Three parts: the reported symptom is fixed in the consumer, and the generator defects the investigation turned up are fixed in the data pipeline.
The published dataset goes from 1,353 to 1,363 SKUs — no row removed, ten added by a wider region sweep. Schema unchanged at three columns.
Part 1 — Reservations on retired SKUs disappeared from the reports
The reservation workbooks join the ISF open data with
kind=inner. That data comes from the Catalogs API, which only returns SKUs you can still purchase — so an active reservation on a size Azure no longer sells has no row to match and is dropped from the report entirely. Not shown at zero utilization: absent. The customer keeps paying for a reservation the tool stops mentioning.All seven ISF joins in
reservations-usage.jsonbecomeleftouter, with the group and ratio falling back to the SKU itself:The
coalesceis not optional. A plainleftouteris worse than today:RatioandISFGroupcome back null,TotalReservedQuantity_s * Ratiobecomes null, andsummarize ... by ISFGroupcollapses every unmatched reservation into a single null bucket with an empty utilization column.Ratio = 1is correct here.Util7Days_scomes from Azure and already accounts for instance size flexibility, so the percentage is right as-is. The ratio only converts between group units, a no-op for a group of one.reservations-potential.jsonandbenefits-simulation.jsonkeepkind=inner. They model what to buy, and there restricting to purchasable SKUs is correct.Known limitation. Drilldowns fall back to the consumed size while the overview falls back to the reserved size. For a reservation on
Standard_DS4_v2runningDS3_v2underneath, those don't meet — which sizes belong to a retired group is exactly what the ISF data no longer carries. A drilldown on an unmatched reservation therefore shows usage of the identical SKU only. Better than nothing, not a full restoration.Part 2 — Three generator defects
Region coverage. The sweep used a hardcoded list of 26 regions, so SKUs that launch in only a handful of regions never entered the dataset, and the list had to be hand-maintained. It now enumerates every physical region from the ARM locations API — 63 today. Note
regionTypeis nested undermetadata, not at the top level, and 46 of the 109 entries are logical groupings (global,unitedstates,europe) that are not valid catalog scopes. An empty enumeration fails the run rather than sweeping a partial set. Measured: 7.9 minutes for 63 regions × 3 reserved types, against a 60-minute timeout.Culture handling. Ratios were parsed and written with the current culture:
TryParse("2.1")Export-Csvwrites2.1"2.1"21"2,1""2,1"CI runs on
ubuntu-latestand was never affected, but the README documents running the generator by hand, so a contributor in a comma-decimal locale would have published a corrupt file —"2,1"breaks theexternaldata(... Ratio:double)joins and both Power BI partitions typed asnumber. Both parse sites and the write path now pinInvariantCulture.Zero ratios. The API returns
azure_redis_cache_isolated_i100with a ratio of0, andbenefits-simulation.jsondivides byRatio. Non-positive ratios are dropped, with a unit test asserting the invariant on the published file.Part 3 — Ratios were on the wrong scale
The Catalogs API leaves ratios unnormalized: only 52 of its 211 groups have a smallest SKU of
1. Microsoft's documentation calls this out —BS Seriesstarts at0.25,Ddsv5 Seriesat2— and publishes a normalization step of its own. The retiredisfratioblob.csvthis dataset replaced was normalized, so #2222 silently changed the scale under every consumer.That matters because the Optimization Engine converts quantities into units of a group's smallest SKU —
AvgRIsUsedInSmallestRatiois the variable's own name — and multiplying by a ratio that doesn't start at1inflates the absolute figures by a per-group constant. Utilization percentages carry the factor on both sides and were never wrong; the displayed quantities were.It also makes Part 1 coherent. An unmatched reservation falls back to
Ratio = 1, which is implicitly "smallest SKU = 1"; next to an unnormalized group that would put two rows of one table in different units.Each group is now normalized.
-Normalizeis replaced by-Raw, so the switch names the departure from the default rather than the default itself.Parity with the retired file is close but not total, and the gap is worth understanding rather than glossing. Of the 1,291 rows the two files share, 1,223 now agree. The 68 that don't split cleanly:
BS Series High Memoryis down toStandard_B20msalone, which normalizes to1rather than40.2— internally consistent, since no other SKU of that group remains.Measured impact on consumers: Power BI is unaffected —
x_CommitmentDiscountFlexRatioappears in no DAX measure and no report visual, only in linguistic metadata; the visuals use the flexibility group. The Optimization Engine's absolute quantities change, which is the point.What this deliberately does not do
An earlier revision backfilled 1,220 retired SKUs from the ratio file the API replaced. That was dropped. Those rows would be frozen forever — nothing can refresh or correct them once the source blob is gone — inside a dataset whose contract is a weekly refresh from the authoritative API. Fixing the consumer avoids carrying dead reference data.
Fixes #2309 as part of this.
Test plan
Invoke-Pester ./src/powershell/Tests/Unit/* ./src/powershell/Tests/Lint/*— 6,010 passed, 0 failed (4 pre-existing skips)isfratioblob.csv: 1,223 of 1,291 shared rows agree, the rest classified above1, asserted by a unit testde-DEround tripThe workbook change is reasoned, not verified. No test in this repo executes workbook KQL, and I have no Optimization Engine workspace with reservation data to run it against. The join semantics and the null behaviour that motivates the
coalescewant confirming by someone who does.🤖 Generated with Claude Code